Add labels to create run /list runs api.#7430
Conversation
afbrock
force-pushed
the
adam/labels-phase-one
branch
2 times, most recently
from
6a5f47c to
faff232
Compare
afbrock
force-pushed
the
adam/labels-phase-one
branch
4 times, most recently
from
3c91886 to
a9c32ab
Compare
| // execution time, also configurable via settings). On create, labels supplied on | ||
| // the request are merged with settings/RunSpec labels, with request labels taking | ||
| // precedence, and the merged set is stored here and on the RunSpec. | ||
| map<string, string> labels = 2; |
There was a problem hiding this comment.
Why do we need it here? RunDetails message has the RunSpec which has Labels
There was a problem hiding this comment.
It needs to be in this one because a list request returns Run not RunDetail.
There was a problem hiding this comment.
ah I see... is there a Label type to use here 👇🏽 ?
afbrock
force-pushed
the
adam/labels-phase-one
branch
3 times, most recently
from
08277db to
c206519
Compare
| enum LabelSelectorOperator { | ||
| LABEL_SELECTOR_OPERATOR_UNSPECIFIED = 0; | ||
| LABEL_SELECTOR_OPERATOR_IN = 1; | ||
| LABEL_SELECTOR_OPERATOR_NOT_IN = 2; | ||
| LABEL_SELECTOR_OPERATOR_EXISTS = 3; | ||
| LABEL_SELECTOR_OPERATOR_DOES_NOT_EXIST = 4; | ||
| } | ||
|
|
||
| message LabelSelectorRequirement { | ||
| string key = 1; | ||
| LabelSelectorOperator operator = 2; | ||
| repeated string values = 3; | ||
| } |
There was a problem hiding this comment.
Can we not use filter in common.ListRequest
https://github.com/unionai/cloud/blob/8aab6e4ffbd883231790bb22a8f658e0564906c4/gen/pb-go/common/list.pb.go#L308-L318
There was a problem hiding this comment.
no, we can't because we are matching key value pairs, and so the selector can express something like the key 'team' exists, or the key 'team' has values in 'infra' 'frontend' or 'ml'
the operators seem similar, but the requirements are really different than common.list.filters.
There was a problem hiding this comment.
I get that label selector semantics are different because they operate on the labels map, not directly on a top-level field.
But since Flyte runs are already filtered through ListRequest.filters, I’m wondering if labels should still be modeled inside the same filtering abstraction.
Example query:
status = SUCCEEDED
created_by = praful
label team in (infra, frontend, ml)
label env exists
With separate LabelSelectorRequirement, clients now need to split this into two APIs:
filters[]:
status = SUCCEEDED
created_by = praful
label_selectors[]:
team in (infra, frontend, ml)
env exists
Could we instead model this as one filter system, e.g.
filters[]:
status EQUAL SUCCEEDED
created_by EQUAL praful
labels.team VALUE_IN [infra, frontend, ml]
labels.env EXISTS
or add a filter target/scope like FIELD vs LABEL?
That keeps ListRequest as the single query abstraction while still preserving label-specific semantics.
There was a problem hiding this comment.
Ok, yeah I like that - let me double check that it works impl wise. I do like the idea of just using common list but i couldnt see how it could work. the dot separated labels key makes sense.
pmahindrakar-oss
left a comment
pmahindrakar-oss
left a comment
There was a problem hiding this comment.
Can we use the existing ListRequest filter instead of introducing a parallel label selector filtering and the comparision parameter around those
| // the request are merged with settings/RunSpec labels, with request labels taking | ||
| // precedence, and the merged set is stored here and on the RunSpec. | ||
| // It is part of Run so that it gets returned in a list request. | ||
| map<string, string> labels = 2; |
There was a problem hiding this comment.
Could we use Label as type here?
flyte/flyteidl2/task/run.proto
Line 72 in c206519
| // ALL of these key=value pairs must be present (AND semantics). | ||
| map<string, string> match_labels = 1; | ||
|
|
||
| // Set-based requirements — reserved for future use. | ||
| repeated LabelSelectorRequirement match_expressions = 2; |
There was a problem hiding this comment.
If we decided to add this, should we add buf.validate here to limit the max length? We can follow the limit set in Kubernetes
https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
afbrock
force-pushed
the
adam/labels-phase-one
branch
from
c206519 to
eb16c67
Compare
afbrock
force-pushed
the
adam/labels-phase-one
branch
from
eb16c67 to
665c740
Compare
Signed-off-by: Adam Brock <adam@union.ai>
afbrock
force-pushed
the
adam/labels-phase-one
branch
from
665c740 to
458a72a
Compare
4 participants
Tracking issue
https://linear.app/unionai/issue/PRD-422/be-listruns-should-include-run-labels
Why are the changes needed?
These changes add labels to the list runs api. This is done by adding effectively a duplication of the labels that are currently in runspec into the top level run object, as list runs only returns a repeated run object instead of the full
run details object (which is the right thing to do.) The other change is that I added operators specific to (and important for) filtering key value pairs into the common list filters enum. A user can add flyte specific key value pair labels in order to organize and filter their runs by adding filters where the field is the concatenation of 'labels." and the key name.
How was this patch tested?
Along with
flyteorg/flyte-sdk#1140
and https://github.com/unionai/cloud/pull/16161
running in devbox, runs can be created and filtered based on label data that is persisted along with run information.
Check all the applicable boxes
Docs link